Add DisableNativeToolsetInstalls check to tools.ps1/.sh #4133
Conversation
Test Build with the change for WinForms |
eng/common/tools.sh
Outdated
| @@ -4,6 +4,7 @@ | |||
|
|
|||
| # CI mode - set to true on CI server for PR validation build or official build. | |||
| ci=${ci:-false} | |||
| DisableConfigureToolsetImport=${DisableConfigureToolsetImport:-null} | |||
There was a problem hiding this comment.
Format for variable is inconsistent with the rest of the file. Should be something like disable_configure_toolset_import
There was a problem hiding this comment.
Not all seem to follow the pattern - https://github.com/dotnet/arcade/blob/master/eng/common/tools.sh#L50
There was a problem hiding this comment.
That's true, and it's probably too late to fix. Fortunately, it's not too late to update yours to align with the general conventions used in this file.
| @@ -418,6 +420,9 @@ function GetSdkTaskProject([string]$taskName) { | |||
| } | |||
|
|
|||
| function InitializeNativeTools() { | |||
| if ($env:DisableNativeToolsetInstalls) { | |||
There was a problem hiding this comment.
It should be just $null -eq $DisableNativeToolsetInstalls.
There was a problem hiding this comment.
With strict mode on, it should probably be something like
if (-Not( Test-Path variable:DisableNativetoolsetInstalls))
@sunandabalu , can you take a look?
There was a problem hiding this comment.
Looks like Viktor already has a PR. Thanks @ViktorHofer
| @@ -252,6 +253,9 @@ function GetNuGetPackageCachePath { | |||
| } | |||
|
|
|||
| function InitializeNativeTools() { | |||
| if [[ -z "${DisableNativeToolsetInstalls:-}" ]]; then | |||
There was a problem hiding this comment.
@sunandabalu , is this supposed to be -n instead of -z?
There was a problem hiding this comment.
-z to check if its empty, if empty then exit.
There was a problem hiding this comment.
Don't you want to exit if it's not empty?
There was a problem hiding this comment.
For that matter, the fix viktor made seems off as well. What's the intent of "disable" native toolset installs? I interpret your implementation as, if this value is empty, then do nothing, if it is not defined, then skip native tools installs. Something seems broken here, but perhaps the logic is getting jumbled in my head.
Test Build - https://dev.azure.com/dnceng/internal/_build/results?buildId=390365&view=results